Ensure GetCSRF doesn't return an empty token
#32130
Conversation
wolfogre
added
type/testing
skip-changelog
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/XS
|
|
pull-request-size
bot
added
size/S
|
I believe it's worth being backported to follow #32069. |
|
Hmm ... I see the "anonymous" crsf token is provided by "user/login" now .... |
|
I have a new idea to improve it, maybe after this PR. Since it doesn't matter where the test code gets the CSRF token to send requests, why not -func GetCSRF(t testing.TB, session *TestSession, urlStr string) string
+func GetCSRF(t testing.TB, session *TestSession) string {
// try "/user/login" to get token
// if it returns `SeeOther`, so it's logged in, try "/user/settings" again
}
|
That's not the initial idea of Gitea's CSRF token design, see this:
Ideally, the CSRF token was designed to be related to current page (even the current form/action), but nobody did so. |
Oh, thanks, I understand now. |
GiteaBot
added
lgtm/need 1
| @@ -59,15 +57,14 @@ func createAttachment(t *testing.T, session *TestSession, repoURL, filename stri | |||
| func TestCreateAnonymousAttachment(t *testing.T) { | |||
| defer tests.PrepareTestEnv(t)() | |||
| session := emptyTestSession(t) | |||
| // this test is not right because it just doesn't pass the CSRF validation | |||
| createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusBadRequest) | |||
| createAttachment(t, session, GetCSRF(t, session, "/user/login"), "user2/repo1", "image.png", generateImg(), http.StatusSeeOther) | |||
There was a problem hiding this comment.
I don't know if someone looking at this code in two months knows why an attachment test calls a login endpoint...
There was a problem hiding this comment.
It reads as this: when TestCreateAnonymousAttachment, it needs to GetCSRF from /user/login, it sounds reasonable.
There was a problem hiding this comment.
If the CSRF token is not action related why can't we have a GetCSRF method without passed url? If /user/login is a redirect call /user/settings. One of both requests contains a token.
There was a problem hiding this comment.
It is user-related.
And I guess just no one has interest to work on #32130 (comment) , and no one has the explicitly said that "that FIXME could be removed and Gitea will always use a common CSRF token for current user"
There was a problem hiding this comment.
I think what @KN4CK3R wants to say is we should document exactly that with a comment…
There was a problem hiding this comment.
I know it's user-related, that's the reason why there is the session.
func GetCSRF(t testing.TB, session *TestSession) string {
t.Helper()
req := NewRequest(t, "GET", "/user/login")
resp := session.MakeRequest(t, req, NoExpectedStatus)
if resp.Code == http.StatusSeeOther {
req = NewRequest(t, "GET", "/user/settings")
resp = session.MakeRequest(t, req, http.StatusOk)
}
doc := NewHTMLParser(t, resp.Body)
return doc.GetCSRF()
}
If we decide to use action-related CSRF tokens the GetCSRF method with an url must change too because then a single page can contain multiple forms with different tokens but doc.GetCSRF() currently just returns the first token.
There was a problem hiding this comment.
It would slow down many tests by using the extra request.
In most cases, the test writer clearly knows what CSRF token they would like to use: anonymous, or a signed-in user.
If you prefer to do so, I think it needs 2 functions:
GetCSRFForAnonymous
GetCSRFForCurrentUser
There was a problem hiding this comment.
(while these improvements could be in a separate PR but not in this PR's scope)
GiteaBot
added
lgtm/done
lunny
added
the
reviewed/wait-merge
Since page templates keep changing, some pages that contained forms with CSRF token no longer have them. It leads to some calls of `GetCSRF` returning an empty string, which fails the tests. Like https://github.com/go-gitea/gitea/blob/3269b04d61ffe6a7ce462cd05ee150e4491124e8/tests/integration/attachment_test.go#L62-L63 The test did try to get the CSRF token and provided it, but it was empty.
GiteaBot
added
backport/done
Backport #32130 by @wolfogre Since page templates keep changing, some pages that contained forms with CSRF token no longer have them. It leads to some calls of `GetCSRF` returning an empty string, which fails the tests. Like https://github.com/go-gitea/gitea/blob/3269b04d61ffe6a7ce462cd05ee150e4491124e8/tests/integration/attachment_test.go#L62-L63 The test did try to get the CSRF token and provided it, but it was empty. Co-authored-by: Jason Song <i@wolfogre.com>
* giteaofficial/main: Fix javascript error when an anonymous user visiting migration page (go-gitea#32144) Make oauth2 code clear. Move oauth2 provider code to their own packages/files (go-gitea#32148) Support repo license (go-gitea#24872) Fix the logic of finding the latest pull review commit ID (go-gitea#32139) Ensure `GetCSRF` doesn't return an empty token (go-gitea#32130) Bump minio-go to latest version (go-gitea#32156)
Since page templates keep changing, some pages that contained forms with CSRF token no longer have them.
It leads to some calls of
GetCSRFreturning an empty string, which fails the tests. Likegitea/tests/integration/attachment_test.go
Lines 62 to 63 in 3269b04
The test did try to get the CSRF token and provided it, but it was empty.